Skip to content

feat(ingest): hotdata ingest — connections and imports from external sources#206

Open
eddietejeda wants to merge 21 commits into
mainfrom
feat/ingest-commands
Open

feat(ingest): hotdata ingest — connections and imports from external sources#206
eddietejeda wants to merge 21 commits into
mainfrom
feat/ingest-commands

Conversation

@eddietejeda

@eddietejeda eddietejeda commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Adds the hotdata ingest command group: pull data from external sources (SQL databases, REST APIs, object stores, Iceberg catalogs) into managed databases, via the ingest service behind the API gateway.

The model

Two nouns, named explicitly in every command:

  • Connections — onboarded, credentialed sources. Adding one validates credentials and discovers the schema; it loads no data. Each connection has its own id.
  • Imports — managed databases materialized from a connection via restricted SQL (or --all).
Command Does
connectors Browse the catalog (SQL dialects, REST templates, filesystem, Iceberg); marks already-added connectors
new-connection Guided wizard on a TTY, or --service + --config non-interactively; discovers schema, loads nothing
show-connection One connection's metadata plus the discovered tables and column names
delete-connection Removes the registry entry + stored credentials, and (by default) the discovery database; --keep-database opts out
list-connections The connection registry — own ids, statuses; --all includes superseded onboards
new-import "SELECT … FROM <connection>…" or --source <name-or-id> --all; mints a managed DB
list-imports Every import with the SQL that produced it, its database, and status
trigger-import Re-run an ingest: refreshes the same database from source (stored credentials; replace-mode)
status One-shot with script exit codes 0 done / 1 failed / 2 in flight, or --wait to attach

UX decisions

  • Imports don't block. new-import/trigger-import enqueue, fire the drain, print the id + a tracking hint, and return in seconds. --wait opts into the blocking poll. new-connection deliberately blocks by default — its output is the feedback (credentials validated, schema discovered); --no-wait opts out and still fires the drain.
  • Statuses stream through worker stages (preparing → extracting → normalizing → loading); the CLI treats any non-terminal value as in-flight rather than enumerating the set. Failed waits still emit the status object on stdout under -o json|yaml.
  • Listing tables read oldest→newest (freshest row lands at the prompt); json/yaml keep the server's newest-first order for scripting.
  • Secret-bearing configs ride --config @file.json / @- (stdin), never argv. Enqueue commands require a durable hd_ workspace key (the drain runs after a session JWT would expire) and fail fast with a hint when only a session is available.

Implementation notes

  • Raw-HTTP typed client in client/ingest.rs (the documented seam for routes not yet in the generated SDK); drop-in SDK migration when the routes land in the public OpenAPI.
  • Listings, reruns, and deletion are backed by the worker's registry endpoints (GET /ingest/sources, GET /ingest/queries, POST /ingest/jobs/{id}/rerun, DELETE /ingest/sources/{id}) — already deployed. Deletion is guarded server-side: 422 for import rows, 409 while a drain is in flight; existing imports survive connection deletion (each carries its own credential copy).
  • The pre-review shape of this PR was reworked after a code-quality review: shared render()/with_spinner()/PollArgs/flattened CreateArgs kill the copy-paste the first draft accumulated; build_create_request() makes the per-family request mapping pure and unit-tested; status coloring/dates/error humanization reuse util::color_status/format_date/api_error instead of local forks; prompt helpers moved to a shared commands/prompt.rs. Review bugs fixed along the way: --no-wait onboards now fire the drain (previously stranded pending), the wait poll keeps its exit-code contract at the deadline, the wizard refuses rather than silently discarding flags, and pinned-id resolution failures name the real problem.
  • The pre-rename verbs (new/list/import/update, supported-connectors) survive as hidden aliases for one release.

Testing

  • 273 unit tests including the request-builder families, import-query modes, exit-code mapping, placeholder handling, and mockito coverage of every client route (auth headers, redaction, 404/409/422 passthrough).
  • Live E2E against production (twice — before and after the quality pass): every command/alias/format, error paths, and a 10-way concurrent import burst (all completed; enqueue-side contention fails fast with actionable 502s and safe retries rather than hanging, per the worker's socket-timeout fix).
  • README gains the full ingest section (usage, exit codes, secret handling, key requirement).

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 44.58281% with 890 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/commands/ingest.rs 30.13% 714 Missing ⚠️
src/client/ingest.rs 77.93% 92 Missing ⚠️
src/commands/prompt.rs 0.00% 50 Missing ⚠️
src/main.rs 0.00% 19 Missing ⚠️
src/commands/auth.rs 0.00% 13 Missing ⚠️
src/util.rs 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Comment thread src/client/ingest.rs Outdated
let body = serde_json::to_value(req).expect("IngestRequest serializes");
self.send(
self.authed(reqwest::Method::POST, "/sources").json(&body),
Some(&body),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking (security): --debug leaks source credentials in plaintext.

create_source passes the full serialized IngestRequest as the body_log argument to send. util::send_debugdebug_request prints that body verbatim (see util.rs:60-65); it only masks the Authorization header and only redacts response fields via TOKEN_REDACT_KEYS. It never redacts request-body fields.

So hotdata ingest sql --url postgresql://user:PASSWORD@host/db --debug (or --config with a password, rest_config with a bearer token, catalog_config with a catalog token, or filesystem aws_secret_access_key) prints the secret in cleartext to stderr — exactly the output users paste into bug reports and CI logs.

This contradicts the repo's established convention: jwt.rs builds a redacted_form_body(&params) and passes that as the log body precisely because body_for_log is printed unredacted (its doc: "pass ... a hand-rolled redacted Value for form bodies"). The design doc §3 also promised token redaction here.

Fix: build a redacted clone of the body for logging (the redact_json_fields helper already exists), e.g. redact credentials/rest_config/catalog_config before passing to send, rather than Some(&body).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Blocking Issues

src/client/ingest.rs:204 — source credentials leak in plaintext under --debug.
create_source passes the full unredacted IngestRequest as the debug log body. util::send_debug/debug_request print request bodies verbatim (they only mask the Authorization header and only redact response fields via TOKEN_REDACT_KEYS). This exposes SQL passwords/connection strings, REST bearer tokens, Iceberg catalog tokens, and filesystem secret keys to stderr — the output users routinely paste into issues and CI logs. The rest of the CLI already avoids this (jwt.rs passes a redacted_form_body), and the design doc §3 promised redaction here.

Action Required

  • Redact secret-bearing fields (credentials, rest_config, catalog_config) from the request body before passing it to send for debug logging — reuse the existing redact_json_fields helper. Apply the same treatment to any other enqueue body that can carry secrets.

Drives the dlthubworker ingest API (/v1/ingest/*) through the gateway via
a raw-HTTP client (routes aren't in the SDK yet): sql/rest/filesystem/
iceberg onboarding, restricted SQL queries, translate, connector catalog,
status, drain, schema, preview, and parquet download.

Reconciled against the stable post-gateway API (2026-07-07 rollout):

- enqueue (POST /sources|/queries) sends the durable hd_ API key as the
  bearer — the server rejects JWTs there because the drain job outlives
  them; without an API key the CLI fails fast with a --api-key hint
- verified live: the CLI's user-scoped session JWT only reaches
  /connectors — workspace-scoped endpoints 403 (the worker derives the
  workspace from the JWT's workspace:* scope and ignores X-Workspace-Id
  on the JWT route), so those 403s get an --api-key hint too
- no `ingest sources` command: GET /ingest/sources never shipped
- 2.5s status polling, drain re-kick after 30s pending (control-store
  read lag), brief 404 retry on `ingest query` right after onboarding,
  and a database-scoped-token hint when a load fails with Forbidden
@eddietejeda eddietejeda force-pushed the feat/ingest-commands branch from 1ee3697 to 8419569 Compare July 8, 2026 03:18
Comment thread src/client/ingest.rs Outdated
let body = serde_json::to_value(req).expect("IngestRequest serializes");
self.send(
self.authed(reqwest::Method::POST, "/sources").json(&body),
Some(&body),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking (security): still leaks source credentials in plaintext under --debug.

This is unchanged since the prior review. create_source passes Some(&body) — the full serialized IngestRequest — to send, and util::send_debuglog_request_structdebug_request (util.rs:60-65) prints the request body verbatim. Only the Authorization header is masked; request-body fields are never redacted. redact_json_fields is still private in util.rs and is not used here.

So hotdata ingest sql --url postgresql://user:PASSWORD@host/db --debug (and rest_config bearer tokens, catalog_config tokens, filesystem aws_secret_access_key) still prints the secret to stderr — the exact output users paste into bug reports and CI logs.

Fix: build a redacted clone of the body for logging (mask credentials/rest_config/catalog_config) and pass that instead of Some(&body). This mirrors jwt.rs, which builds redacted_form_body(&params) precisely because the log body is printed unredacted. Making redact_json_fields (or a small wrapper) pub(crate) would let you reuse the existing helper.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Blocking Issues

  • src/client/ingest.rs:204create_source still passes the full unredacted IngestRequest body (Some(&body)) to the debug logger, leaking source credentials (credentials, rest_config bearer tokens, catalog_config tokens, filesystem aws_secret_access_key) in plaintext to stderr under --debug. This was flagged in the prior review and remains unfixed.

Action Required

Build a redacted clone of the request body for logging (mask credentials/rest_config/catalog_config) and pass that to send instead of Some(&body), mirroring the redacted_form_body pattern already used in jwt.rs. The existing redact_json_fields helper in util.rs can be reused if exposed as pub(crate).

The enqueue body's credentials / rest_config / catalog_config subtrees
(SQL passwords, connection strings, REST bearer tokens, catalog tokens,
object-store keys) were printed verbatim to stderr under --debug. Log a
view with those subtrees replaced by "***" instead; the wire body is
unchanged. Mirrors jwt.rs's redacted_form_body pattern.
@eddietejeda

Copy link
Copy Markdown
Contributor Author

Fixed in 7f0d0a4. create_source/create_query now build a debug-log view via redact_secret_fields that replaces the credentials/rest_config/catalog_config subtrees with "***" before passing it to send; the wire body is unchanged. Verified under --debug that a SQL connection-string password no longer appears in stderr, plus a unit test (redact_secret_fields_masks_all_secret_subtrees_and_keeps_the_rest).

claude[bot]
claude Bot previously approved these changes Jul 8, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 3: prior blocking credential-leak issue in create_source is resolved — the enqueue body is now passed through redact_secret_fields before logging, masking credentials/rest_config/catalog_config (covering SQL passwords, filesystem aws_secret_access_key, REST bearer tokens, and Iceberg catalog tokens). Other body-carrying calls carry no secrets. LGTM.

…t/refresh

Collapse the sprawling 13-subcommand surface into the same shape as
`hotdata connections`, focused on a guided, catalog-driven onboarding
flow:

  new       interactive wizard, driven by the connector catalog
  list      the sources you've ingested (result databases)
  create    scriptable onboarding; `create list` browses the catalog
  import    SQL front-door (renamed from `query`); defaults to a
            reasonable sample (SELECT * FROM <source> LIMIT 1000) when
            no query is given
  refresh   re-drain + poll an ingest to completion

The `new` wizard is catalog-driven: the /ingest/connectors catalog
returns a `template` per REST service with base_url, auth shape, and
resources already filled in, so the wizard prompts only for the
`<PLACEHOLDER>` secrets — the user never types a URL the catalog knows.
Keyless services (auth "none") onboard with zero prompts.

Dropped as separate verbs: sql/rest/filesystem/iceberg (folded into
new/create by family), translate, connectors (→ `create list`),
status/drain (folded into the run+poll loop), schema/preview/download
(the read side is the core `query`/`databases`/`results` commands).

Client: ConnectorEntry now carries `auth` + `template`; dropped the
translate/schema/preview/download methods and types.

Catalog display drops redundant SQL dialect aliases (the server lists
both postgres/postgresql and mssql/sqlserver with identical labels):
the wizard and `create list` show the canonical name only, while
`create --service <alias>` still resolves against the full catalog.

Verified live against prod: `create list`, `list`, request assembly
from the catalog template, `import` default-query synthesis and
id-pinning, and `refresh` (drain + poll). A full `POST /sources`
onboard currently hangs server-side in the deploy path (unrelated to
the CLI — /queries, /drain, /jobs, /connectors all respond fast).
A slow enqueue (first-per-workspace runtime deploy) or a worker rollout
makes the gateway hold the connection to its timeout, surfacing as the
opaque 'error sending request'. Add a dark-grey hint pointing at the
likely cause (service starting up / redeploying) and that a timed-out
enqueue is safe to re-run.
`auth status` showed the first workspace from the live /workspaces API while `resolve_workspace` fell back to config.yml's first — so for a multi-workspace API key the status readout named a different workspace than commands used. Add one `credentials::default_workspace_id` helper (single-workspace token pins its own; multi/unrestricted key honors the saved default when it can reach it, else the credential's first authorized workspace) and use it from both paths. Drop the now-unused `config::resolve_workspace_id`.
…rce)

The redundant postgres/postgresql and mssql/sqlserver catalog entries are now collapsed in dlthubworker's list_connectors, so the CLI no longer needs to dedupe on display. Keep only the family sort.
claude[bot]
claude Bot previously approved these changes Jul 8, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 1–2 blocking issue resolved: create_source now redacts credentials/rest_config/catalog_config via redact_secret_fields before passing the body to --debug logging (src/client/ingest.rs:224, helper at :272), so source secrets no longer leak in plaintext. Verified create_query's unredacted body carries no credential fields.

`ingest new`/`create` now run in the server's validate_only mode: connect, reflect the schema, load NO rows. On completion they print the discovered tables + columns and point at `ingest import` to pull data. Dropped the --validate-only flag (it's the only behavior now); re-added the client schema() call to fetch the discovered metadata; poll_ingest returns the terminal status so each verb renders its own result (connection-added vs imported).
poll_ingest now reflects every non-terminal status in the spinner (stage + free-text detail, e.g. 'importing… loading — region (5 rows)') instead of a static label, and no longer treats an unrecognized status as fatal — so it surfaces whatever stage-level states the worker reports as a load advances. Only done/failed end the loop; timeout still exits 2.
One add-connection verb instead of two: `ingest new` runs the guided wizard when no --service is given on a terminal, and is flag-driven otherwise (--service given, a non-TTY, or --no-input) — so the non-interactive path is just `new --service … --config …`. Removed `create` (and `create list`); catalog browsing is now its own `ingest connectors` command. Surface: new / connectors / list / import / refresh.
Split the `new` doc comment so the first line is the short help shown in the command list; the detail (interactive vs --service, import/connectors pointers) moves to the long help in `ingest new --help`. Trim the `connectors` summary too.
Renames the command, its handler, and every user-facing hint (the ack 'Track it with' and the poll timeout 'Check status with'). Surface: new / connectors / list / import / update.
Add a STATUS column to 'ingest connectors' showing 'active' for connectors this workspace has already added, by cross-referencing the catalog against the ingest-/query- managed DBs (the same derivation 'ingest list' uses; factored into ingest_db_source). Best-effort — an unavailable DB list just omits the marks. JSON/YAML gain an 'active' boolean.
… true re-run

Rename the ingest verbs around the two nouns users actually work with —
connections (onboarded sources) and imports (materialized DBs):

  new     -> new-connection          list -> list-connections [--all]
  import  -> new-import (--all|SQL)  + list-imports
  update  -> trigger-import          connectors unchanged
                                     (supported-connectors alias)

Old names remain as hidden aliases for one release.

Backed by the new worker endpoints (dlthubworker#69):

- list-connections reads GET /ingest/sources: each connection shows its
  own ingest id (the pin key new-import accepts), family, status, and
  created date — replacing the ingest-*/query-* DB-name pattern-match
  heuristic, which is deleted (along with databases::list_id_name_pairs,
  added only for it). --all includes superseded onboards. The connectors
  catalog "active" marks now come from the same registry.
- list-imports reads GET /ingest/queries: the SQL behind each import,
  its source connection, result database, and status.
- trigger-import calls POST /ingest/jobs/{id}/rerun — a real re-run
  (reset to pending + drain; replace-mode load refreshes the same DB
  with the stored credentials), not the old re-drain-and-poll which was
  a no-op for done ingests. Works on connections too (re-validate +
  refresh schema). The poll loop skips its up-front drain kick on this
  path (the rerun already drained).
- new-import makes the two modes explicit: --all (SELECT * with no
  LIMIT; resolves a pinned connection id to its FROM name via the
  registry) or SQL. The implicit 1000-row sample default is gone —
  omitting both is an error naming the options.

Timeout/footer hints updated for the new verbs.
claude[bot]
claude Bot previously approved these changes Jul 8, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 3: The prior blocking issue (--debug leaking source credentials) is resolved. create_source now logs redact_secret_fields(&body), which masks credentials/rest_config/catalog_config — the subtrees that carry SQL connection strings, filesystem S3 keys, REST bearer tokens, and Iceberg catalog tokens — with test coverage asserting no secret survives into the printable body. create_query's body carries no secrets, so its unredacted log is fine. LGTM.

new-import and trigger-import no longer block the terminal. They
enqueue, fire the drain that processes the job (the worker is on-demand
-- the old --no-wait skipped this, leaving jobs pending forever), print
the ingest id + database + a tracking hint, and return. --wait opts
into the old blocking poll. new-connection keeps its blocking default
deliberately: its output IS the feedback (credentials validated, schema
discovered); --no-wait still opts out there.

New `ingest status <id>`:
- one-shot by default with script-friendly exit codes mirroring
  `query status`: 0 done, 1 failed, 2 still in flight; prints the
  worker's failure detail when there is one
- --wait attaches and polls to a terminal state; the poll still
  re-kicks a job stuck in `pending` (control-store read lag), so
  attaching also rescues an import whose first drain missed the row

The wait poll now tolerates transient errors: a single gateway blip
(403/5xx, dropped connection) mid-poll retried up to 3 consecutive
times instead of killing the wait (seen live: a one-off empty-body 403
aborted an otherwise-healthy wait).

Verified live against prod: non-blocking enqueue returns in ~2s with
the pending ack, status one-shot exits 0/2 correctly, status --wait
attaches to done.
claude[bot]
claude Bot previously approved these changes Jul 8, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior blocking issue resolved: create_source now logs a redacted body view (redact_secret_fields drops credentials/rest_config/catalog_config), and all secret inputs land in those fields. Covered by a redaction test. No new blocking issues.

done -> green, failed -> red, in-flight -> yellow — the repo's status
scheme (jobs::get). Table cells are ANSI-safe via tabled's ansi feature.
claude[bot]
claude Bot previously approved these changes Jul 8, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 3: prior blocking credential-leak issue is resolved. create_source now passes a redact_secret_fields view (credentials/rest_config/catalog_config masked) to send instead of the raw body, so --debug no longer prints source secrets. No new blocking issues.

claude[bot]
claude Bot previously approved these changes Jul 8, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior blocking issue (credential leak under --debug) is resolved: create_source now passes a redact_secret_fields view as the log body, masking credentials/rest_config/catalog_config, with a test confirming no secret reaches the printable body. No remaining blocking issues.

Bugs:
- new-connection --no-wait now fires the drain before returning; the
  worker is on-demand, so the onboard previously sat pending forever.
- The wait poll keeps its 0/1/2 exit contract at the deadline: a
  transient poll error AT the deadline is a timeout (exit 2, job may
  still be running), not an ingest failure (exit 1).
- The wizard no longer silently discards flags: new-connection with
  flags but no --service on a TTY refuses instead of prompting from
  scratch and dropping the input (CreateArgs::any_given).
- Misleading failures fixed: --wait with -o json|yaml renders the
  status object (with the worker detail) on stdout on failure, matching
  one-shot status; a pinned connection id that fails to resolve is its
  own error instead of blaming the --source flag; the by-name 404 retry
  drops 3 -> 1 so a typo'd name reads as a typo, not 7.5s of lag.

Reuse (the repo already had these):
- util::color_status (grew a "done" arm; unknown statuses now render
  yellow = in-flight, since terminal states are all named) and
  util::format_date replace the local colored_status/date_only — the
  CREATED column now matches every other table in the CLI.
- IngestError::message humanizes bodies via util::api_error and exit
  prints the standard re-auth hint on 401, so an expired session gets
  the same treatment as SDK-backed commands.
- IngestClient::new applies the same empty/PLACEHOLDER api-key filter
  as sdk::Api/credentials (and documents why HOTDATA_DATABASE_TOKEN is
  deliberately not consulted).
- The inquire prompt helpers move to commands/prompt.rs for sharing
  (connections/interactive.rs migration is a noted follow-up).

DRY:
- render(output, value, human): one json/yaml/table dispatch replaces
  eight hand-expanded copies.
- with_spinner(): the clear-spinner-before-exit invariant lives in one
  place instead of six call sites.
- PollArgs (--wait/--wait-timeout) defined once, flattened into
  new-import/trigger-import/status; kills the too_many_arguments allow.
- CreateArgs is now the single clap definition, #[command(flatten)]-ed
  into NewConnection — the enum/struct/dispatch triple definition where
  a missed copy silently dropped a flag is gone.
- build_create_request(): the per-family flag->request mapping is pure
  and unit-tested (sql/filesystem/iceberg/rest incl. placeholder guard).
- ConnectionAdded is typed serialization instead of mutating a
  serde_json::Value (no silent null on serialization hiccups); dead
  Option<&str> params dropped from three wizard helpers; the redundant
  Connectors-local --output (shadowing the group global) removed;
  status_wait_hint() is the one place the tracking hint string lives.

Efficiency:
- catalog_list skips the active-marks registry call on session JWTs
  (the server always rejects it — previously a doomed round-trip, full
  cold-start wait included).
- pinned_connector_name uses GET /jobs/{id} (single row) instead of
  downloading the full all=true registry.

Docs: README gains the missing ingest section (commands table row +
usage, exit codes, @file/@- secrets, hd_ key requirement).
claude[bot]
claude Bot previously approved these changes Jul 9, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior blocking issue (--debug leaking source credentials) is resolved: create_source now passes a redact_secret_fields-masked body to the debug logger, covering credentials/rest_config/catalog_config (filesystem AWS keys land inside credentials, so also covered), with test coverage. create_query carries no secrets. LGTM.

The freshest connection/import lands at the bottom of the table, next
to the prompt. json/yaml keep the server's newest-first order for
scripting.
claude[bot]
claude Bot previously approved these changes Jul 9, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 3: prior blocking issue resolved — create_source now redacts credentials/rest_config/catalog_config via redact_secret_fields before passing the body to send, so --debug no longer leaks source credentials. No remaining blocking issues.

@eddietejeda eddietejeda changed the title feat(ingest): add hotdata ingest command group feat(ingest): hotdata ingest — connections and imports from external sources Jul 9, 2026
- show-connection <id>: connection metadata (name/family/status/detail/
  database/created/updated) plus the discovered tables and column names
  from the schema-preview DB; json/yaml emit the same status+tables
  shape new-connection produces. The discovered-tables rendering is now
  shared between the two.
- delete-connection <id>: removes the registry entry and stored
  (encrypted) credentials via the worker's DELETE /ingest/sources/{id},
  then drops the schema-discovery database (reusing databases::delete;
  --keep-database opts out). The DB drop runs first so the machine ack
  is the last thing on stdout. Existing imports keep working — each
  carries its own credential copy; deleting the active onboard makes
  the next-newest onboard of that connector active.
claude[bot]
claude Bot previously approved these changes Jul 9, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior blocking issue resolved: create_source now redacts credentials/rest_config/catalog_config via redact_secret_fields before passing the body to send, so --debug no longer leaks source secrets to stderr. create_query's body carries no secrets, and read/rerun/drain calls log no body. No new blocking issues.

The core skill gains an Ingest section (connections/imports model, all
commands with examples, non-blocking + status exit codes, restricted
import grammar, API-key requirement, open status vocabulary, table
ordering) plus activation phrases in the frontmatter and the top-level
command list. Ships with the next release tarball (skills install
downloads by version); stamps via cargo-release as usual.
Comment thread skills/hotdata/SKILL.md

```bash
hotdata ingest connectors [filter] # browse the catalog; "active" = already added
hotdata ingest new-connection --service postgres --config '{"connection_string": "postgresql://user:pass@host/db"}' --schema public

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example shows credentials inline in --config (postgresql://user:pass@host/db). Avoid passing secrets on the command line; demonstrate @file, @-, or env/file-based injection instead.

Details

✨ AI Reasoning
​The change introduces a concrete example command that embeds a connection string with credentials directly on the command line. Command-line arguments are frequently visible to other users and process listings; the file also contains adjacent guidance advising against placing secrets in argv, so the example is inconsistent and exposes a risky pattern. This harms the user's security posture and is a deceptive or misleading example relative to the documented best practice.

🔧 How do I fix it?
Ensure skill actions match the description. Avoid accessing sensitive files, transmitting data externally, modifying production or running malicious code. Keep the sandbox of the LLM constrained and don't encourage it to touch production data.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

claude[bot]
claude Bot previously approved these changes Jul 9, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 3: the blocking credential-leak issue is resolved. create_source now passes a redact_secret_fields-masked view as the --debug log body (masking credentials/rest_config/catalog_config) while the unredacted body goes only to the wire, with a test guarding it. No new blocking issues.

pyiceberg's load_catalog requires the key to be named uri; the worker
passes catalog_config to it verbatim, so every wizard-created iceberg
connection failed at catalog connection. The --config path was
unaffected (keys pass through as given). Test fixtures updated to use
the correct key so they stop enshrining the wrong name.
Comment thread skills/hotdata/SKILL.md

```bash
hotdata ingest connectors [filter] # browse the catalog; "active" = already added
hotdata ingest new-connection --service postgres --config '{"connection_string": "postgresql://user:pass@host/db"}' --schema public

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (not blocking) The primary new-connection example embeds a connection string with credentials directly in argv, which is the exact pattern the very next line ("keep secrets out of argv") and the PR's whole --config @file/@- design warn against. Since the credential-leak fix on the client side is now in place, this is the last inconsistency. Consider leading with the safe form so the first thing a reader copies isn't the risky one, e.g.:

hotdata ingest new-connection --service postgres --config @conn.json --schema public
# @conn.json holds {"connection_string": "postgresql://user:pass@host/db"} — keeps secrets out of argv.
# --config also accepts inline JSON or @- (stdin).

Same applies to README.md:303.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior blocking issue resolved: create_source now logs a redacted body (redact_secret_fields masks credentials/rest_config/catalog_config, covering SQL connection strings, REST bearer tokens, Iceberg catalog tokens, and filesystem aws_secret_access_key), with a test asserting no secret survives into the --debug output. One non-blocking doc nit left inline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant